Panic if we're running with outdated state instead of force-closing #1564
Conversation
Codecov Report
@@ Coverage Diff @@
## main #1564 +/- ##
==========================================
+ Coverage 91.04% 91.10% +0.06%
==========================================
Files 80 80
Lines 44034 44412 +378
Branches 44034 44412 +378
==========================================
+ Hits 40091 40462 +371
- Misses 3943 3950 +7
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
LGTM, need to fix the build errors reported in https://github.com/lightningdevkit/rust-lightning/runs/7032273201.
| return Err(ChannelError::CloseDelayBroadcast( | ||
| "We have fallen behind - we have received proof that if we broadcast remote is going to claim our funds - we can't do any automated broadcasting".to_owned() | ||
| )); | ||
| macro_rules! log_and_panic { |
There was a problem hiding this comment.
Why is a macro needed? Could just use a variable for $err_msg.
There was a problem hiding this comment.
Not directly - format strings have to be an explicit string, they can't be a &'static str or whatever, so I'm not sure how to do this without either repeating the whole string or a macro.
There was a problem hiding this comment.
I believe you can format it first with let err_msg = format!("..", ..) and then use "{}", err_msg in each statement. Or maybe there is a more idiomatic way of using format_args!?
There was a problem hiding this comment.
Hmm, no, format_args doesn't seem to do it either. Do you find the current code particularly unreadable? It seems fine to me, given its the one way to do it without building the full string first.
There was a problem hiding this comment.
Nah, just figured we could do it without using a macro. Given it's gonna panic there's not a huge argument for building the string. Fine either way.
TheBlueMatt
force-pushed
the
2022-06-panic-on-behind
branch
from
7126128
to
5fc55c3
Compare
| return Err(ChannelError::CloseDelayBroadcast( | ||
| "We have fallen behind - we have received proof that if we broadcast remote is going to claim our funds - we can't do any automated broadcasting".to_owned() | ||
| )); | ||
| macro_rules! log_and_panic { |
There was a problem hiding this comment.
I believe you can format it first with let err_msg = format!("..", ..) and then use "{}", err_msg in each statement. Or maybe there is a more idiomatic way of using format_args!?
TheBlueMatt
force-pushed
the
2022-06-panic-on-behind
branch
from
5fc55c3
to
162024b
Compare
|
@TheBlueMatt feel free to squash. |
If a user restores from a backup that they know is stale, they'd like to force-close all of their channels (or at least the ones they know are stale) *without* broadcasting the latest state, asking their peers to do so instead. This simply adds methods to do so, renaming the existing `force_close_channel` and `force_close_all_channels` methods to disambiguate further.
When we receive a `channel_reestablish` with a `data_loss_protect` that proves we're running with a stale state, instead of force-closing the channel, we immediately panic. This lines up with our refusal to run if we find a `ChannelMonitor` which is stale compared to our `ChannelManager` during `ChannelManager` deserialization. Ultimately both are an indication of the same thing - that the API requirements on `chain::Watch` were violated. In the "running with outdated state but ChannelMonitor(s) and ChannelManager lined up" case specifically its likely we're running off of an old backup, in which case connecting to peers with channels still live is explicitly dangerous. That said, because this could be an operator error that is correctable, panicing instead of force-closing may allow for normal operation again in the future (cc lightningdevkit#1207). In any case, we provide instructions in the panic message for how to force-close channels prior to peer connection, as well as a note on how to broadcast the latest state if users are willing to take the risk. Note that this is still somewhat unsafe until we resolve lightningdevkit#1563.
TheBlueMatt
force-pushed
the
2022-06-panic-on-behind
branch
from
162024b
to
caa2a9a
Compare
|
When we receive a
channel_reestablishwith adata_loss_protectthat proves we're running with a stale state, instead of
force-closing the channel, we immediately panic. This lines up with
our refusal to run if we find a
ChannelMonitorwhich is stalecompared to our
ChannelManagerduringChannelManagerdeserialization. Ultimately both are an indication of the same
thing - that the API requirements on
chain::Watchwere violated.In the "running with outdated state but ChannelMonitor(s) and
ChannelManager lined up" case specifically its likely we're running
off of an old backup, in which case connecting to peers with
channels still live is explicitly dangerous. That said, because
this could be an operator error that is correctable, panicing
instead of force-closing may allow for normal operation again in
the future (cc #1207).
In any case, we provide instructions in the panic message for how
to force-close channels prior to peer connection, as well as a note
on how to broadcast the latest state if users are willing to take
the risk.
Note that this is still somewhat unsafe until we resolve #1563.